[Not to Pull. Just discussion] adding some C++11 aspects and a HOT tracer #1
Conversation
| template <typename Args> | ||
| inline std::shared_ptr<Span> start(Args & args) | ||
| { | ||
| return std::shared_ptr<Span>(tracer.start(args), [this](Span *pi) { tracer.cleanup(pi); }); |
There was a problem hiding this comment.
Would be nice to optimize the deleter method somehow (static?)
| typedef NoopTracer ZipkinV1; | ||
| typedef NoopTracer ZipkinV2; | ||
|
|
||
| // Setting up stuff... Would be nice if they weren't that many... |
There was a problem hiding this comment.
quoting the comment
Would be nice if they weren't that many... is there any way
GenericTracerto deduce those from just MyHotTracer?
| #include <tracing_cpp11.h> | ||
|
|
||
| // Now we "upgrade" the entire system to C++11. Yeiii! | ||
| typedef Cpp11Tracer<GlobalTracer> Cpp11GlobalTracer; |
There was a problem hiding this comment.
Naming it just Tracer would make thing even more pretty.
| @@ -0,0 +1,297 @@ | |||
| #ifndef INCLUDED_OPENTRACING_HOT_H | |||
There was a problem hiding this comment.
Disclaimer: I did just some of Span, Context and Tracer. It's not a complete implementation.
| }; | ||
|
|
||
|
|
||
| /** |
There was a problem hiding this comment.
Quoting the comment
VERY TRICKY. Pay attention to const-ness here. Span's context mustn't be const but
extract()'s has to be const. To be properly thought/implemented.
| t2.inject(carrier, *span.span2); | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
What should be the return value here?
|
|
||
| void cleanupImp(const HotOptions<T1,T2>* ob) | ||
| { | ||
| if (ob) |
There was a problem hiding this comment.
Is this necessary? Think can remove those if's.
| // Application code | ||
| auto tracer = Cpp11GlobalTracer::instance(); | ||
|
|
||
| auto span = tracer.start("hi"); |
There was a problem hiding this comment.
this and ctx below are std::shared_ptr's i.e. there's no mem leak here.
|
|
||
| auto span = tracer.start("hi"); | ||
|
|
||
| std::vector<opentracing::TextMapPair> pairs; |
There was a problem hiding this comment.
Nice that this compiles but it likely means we have a bit too much freedom. I guess here's what's most of the discussion about. I guess both ZipkinV1 and ZipkinV2 should support something that serializes to text and http headers. If there's a proprietary IPC extension that ZipkinV1 supported, I guess that ZipkinV2 should support it as well to be backwards compatible, so no harm. If instead we had ZipkinV1 and Jaeger, then I can see that Jaeger might not support the proprietary IPC extension (out of the box). It would be nice to be able to #include a single header file (with potentially infinite uglyness) and extend your Jaeger tracer to support the proprietary IPC. If this isn't possible, I guess the next alternative is to git-clone Jaeger and add support to it. In any case, for as long as Jaeger` doesn't support your custom IPC, the custom IPC infrastructure should be able to pretend it's a standard "HTTP" or "TEXT" type and it should work out of the box with potentially some inferior performance.
| return std::shared_ptr<const SpanContext>(tracer.extract(carrier), [this](const SpanContext *pi) { tracer.cleanup(pi); }); | ||
| } | ||
|
|
||
| static Cpp11Tracer instance() |
There was a problem hiding this comment.
Note that this doesn't return a shared_ptr. It might be better if it did.
|
Hi, |
This adds some draft implementations of a Higher Order Tracer and demonstrate how you can do a C++11 wrapper (that should be provided by OT / shouldn't be app-specific).